Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the table for listeners to distinguish different message types. #188

Merged
merged 5 commits into from
Jul 17, 2024

Conversation

evshary
Copy link
Contributor

@evshary evshary commented Jun 19, 2024

As @PLeVasseur suggested here, add the table mapping for listeners and message type.
This should be helpful for the implementation.

Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@PLeVasseur
Copy link
Contributor

Hey @evshary -- is this generally useful to include in e.g. the uAttributes spec?

That's what I had originally thought

@gregmedd
Copy link
Contributor

gregmedd commented Jul 1, 2024

I agree that this seems like it belongs in the UAttributes (or even better, UUri) spec. It's not zenoh specific.

It would also be really helpful to have a similar table of the allowable values for both source and sink when sending a message.

up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@evshary
Copy link
Contributor Author

evshary commented Jul 2, 2024

@PLeVasseur @gregmedd @sophokles73 Thanks for your suggestion. I thought it was the only issue Zenoh faced, but it seems like we can view it more generically. I've moved it to uattributes.adoc now.

Copy link
Contributor

@stevenhartley stevenhartley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comment

basics/uattributes.adoc Outdated Show resolved Hide resolved
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Copy link
Contributor

@stevenhartley stevenhartley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some comments but what I would also like to see @evshary is another column of the first table that explains the use case for said combination, by that I mean:

src resource_id sink resource_id Publish Notification Request Response Use Case
[8000-FFFE] None V A uE listens for a specific published message
[8000-FFFE] 0 V A uE listens for a response to a specific notification
[1-7FFF] 0 V A uE listens for a response for a specific request
FFFF 0 V V A uE listens for all notifications or responses
FFFF [1-7FFF] V A service listens for requests coming in from anyone to a specific request
[1-7FFF] FFFF V
[8000-FFFE] FFFF V Streamer listens for any notification from anyone
FFFF FFFF V V V Streamer listens for anything that is sent to this device

up-l1/README.adoc Outdated Show resolved Hide resolved
up-l1/README.adoc Outdated Show resolved Hide resolved

| [8000-FFFE] | None | V | | |
| [8000-FFFE] | 0 | | V | |
| 0 | [1-7FFF] | | | V |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a valid usecase for source and sink matching

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the comments below. Please let me know your thoughts.

| [1-7FFF] | 0 | | | | V
| FFFF | 0 | | V | | V
| FFFF | [1-7FFF] | | | V |
| 0 | FFFF | | | V |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another usecase that I think is not valid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the comments below. Please let me know your thoughts.

Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@evshary
Copy link
Contributor Author

evshary commented Jul 8, 2024

@stevenhartley The idea of the table is to distinguish message type from different source and sink resource_id combinations. You gave a good suggestion to add use cases here, which I didn't think of before.

I've updated the table now. I think the FFFF of the resource_id indicates that we don't care about the value.
For the case {0, FFFF}, it should have the same meaning as {0, [1-7FFF]}, while {0, [8000-FFFE]} is not valid.
However, I agree that users might never use the way to listen to the resource_id. The same concept can be applied to index 6 to index 9. We can leave these combinations here (although users never use them) or perhaps we can forbid these combinations to make the table simpler. WDYT?

index source resource_id sink resource_id Publish Notification Request Response Use Case
1 [8000-FFFE] None V A uE listens for a specific published message
2 [8000-FFFE] 0 V A uE listens for a specific notification message
3 0 [1-7FFF] V A uE listens for a specific request
4 [1-7FFF] 0 V A uE listens for a specific response
5 FFFF 0 V V A uE listens for all notifications and responses
6 FFFF [1-7FFF] V Same as {0, [1-7FFF]}
7 0 FFFF V Same as {0, [1-7FFF]}
8 [1-7FFF] FFFF V Same as {[1-7FFF], 0}
9 [8000-FFFE] FFFF V Same as {[8000-FFFE], 0}
10 FFFF FFFF V V V uStreamer listens for all notifications, requests, and responses

@stevenhartley
Copy link
Contributor

IMHO the table is just getting too complicated. We should only list the known examples that uEs (and/or the streamer) should use and then get rid of any of the combinations that don't make any sense.

@evshary
Copy link
Contributor Author

evshary commented Jul 9, 2024

Then the table can be simplified as

index source resource_id sink resource_id Publish Notification Request Response Use Case
1 [8000-FFFE] None V A uE listens for a specific published message
2 [8000-FFFE] 0 V A uE listens for a specific notification message
3 0 [1-7FFF] V A uE listens for a specific request
4 [1-7FFF] 0 V A uE listens for a specific response
5 FFFF 0 V V A uE listens for all notifications and responses
6 FFFF FFFF V V V uStreamer listens for all notifications, requests, and responses

These combinations only make sense to users, and we can forbid users from using other combinations explicitly.
If you agree with this, then I'll update the table.

@stevenhartley
Copy link
Contributor

Then the table can be simplified as

index source resource_id sink resource_id Publish Notification Request Response Use Case
1 [8000-FFFE] None V A uE listens for a specific published message
2 [8000-FFFE] 0 V A uE listens for a specific notification message
3 0 [1-7FFF] V A uE listens for a specific request
4 [1-7FFF] 0 V A uE listens for a specific response
5 FFFF 0 V V A uE listens for all notifications and responses
6 FFFF FFFF V V V uStreamer listens for all notifications, requests, and responses
These combinations only make sense to users, and we can forbid users from using other combinations explicitly. If you agree with this, then I'll update the table.

@evshary I'm ok with the latest changes. Please update the PR and I will approve it

Signed-off-by: ChenYing Kuo <evshary@gmail.com>
up-l1/README.adoc Show resolved Hide resolved
up-l1/README.adoc Show resolved Hide resolved
@stevenhartley stevenhartley self-requested a review July 16, 2024 17:52
Copy link
Contributor

@stevenhartley stevenhartley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@evshary evshary merged commit ca8172a into eclipse-uprotocol:main Jul 17, 2024
1 check passed
@evshary evshary deleted the update_zenoh_spec branch July 17, 2024 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants